-
-
Notifications
You must be signed in to change notification settings - Fork 170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
URL.build()
: Raise TypeError
if host
argument is None
#809
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I there are valid cases for not having a hostname. For example, this is how one would represent local filesystem paths or URLs, or UNIX file socket URLs, or abstract socket URLs. Hostnames are needed where a URI would represent an IP-network-accessible resource but a generic case would include things like http+unix:///var/tmp/aiohttp-server.sock
even for web apps that are intended to operate locally. YARL is a generic lib so we shouldn't ignore use-cases that exist outside web. There's file:///etc/hosts
or git+ssh://../another-repo/.git
that are valid URLs. And there are URLs that are used in protocols targeting xdg-open
/MIME thingies. Like steam://some-app?params
or twitter://username
etc.
If we want to force people pass a non-None
host, we should at least make sure there are tests for these cases allowing the end-users to construct corresponding URLs with something like an empty string, for example.
But I guess having a |
Codecov Report
@@ Coverage Diff @@
## master #809 +/- ##
=========================================
Coverage ? 99.34%
=========================================
Files ? 4
Lines ? 768
Branches ? 176
=========================================
Hits ? 763
Misses ? 4
Partials ? 1
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@webknjaz - I agree with everything you said and I ran into this problem when I was trying to build a URL for a local file. The thing is that the same argument could be made for In other words, with the changes here, the method simply becomes more consistent in the treatment of the various components. My attempt here is to help people who run into the same issue to get a meaningful error message, rather than having to dig into the code. |
d82f405
to
720efc9
Compare
@paulpapacz could you research if we would be able to have tests for those cases and maybe submit them first (even if we'll have to mark them as xfail for now)? |
Oh, and one other case is relative URIs which only need path+qs. |
Looks like the current situation is that it's not designed for it. At a glance, it looks much the same as the other parameters in that block, and fails if you try to use
I don't think there's any meaningful difference to So, this change looks fine to me. We should get type annotations added though, which would make it more explicit. |
Nevermind, it uses stub files. This is already typed as |
@Dreamsorcerer looks like the CI needs to be fixed before merging this: #803 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dreamsorcerer I'll leave the final merge to you if you don't mind.
Co-authored-by: Sviatoslav Sydorenko <[email protected]>
What do these changes do?
Make
URL.build()
raise aTypeError
ifhost
argument isNone
Are there changes in behavior for the user?
No.
Related issue number
#808
Checklist
CHANGES
folder<issue_id>.<type>
(e.g.588.bugfix
)issue_id
change it to the pr id after creating the PR.feature
: Signifying a new feature..bugfix
: Signifying a bug fix..doc
: Signifying a documentation improvement..removal
: Signifying a deprecation or removal of public API..misc
: A ticket has been closed, but it is not of interest to users.Fix issue with non-ascii contents in doctest text files.